phpmyfaq/src/phpMyFAQ/Faq/Permission.php (1)

58-84: Consider reducing query count for large ID arrays.

The current approach issues 2 queries per ID (SELECT + INSERT). For bulk operations (e.g., 100 IDs → 200 queries), this can be costly. A single SELECT to fetch all existing (record_id, {mode}_id) pairs for the given $faqId upfront, followed by INSERTs only for missing IDs, would cut the query count significantly.

Sketch: batch the existence check

     foreach ($ids as $id) {
-        // Check if permission already exists to avoid duplicate key errors
-        $checkQuery = sprintf(
-            'SELECT 1 FROM %sfaqdata_%s WHERE record_id = %d AND %s_id = %d',
-            Database::getTablePrefix(),
-            $mode,
-            $faqId,
-            $mode,
-            $id,
-        );
-
-        $result = $this->configuration->getDb()->query($checkQuery);
-        if ($this->configuration->getDb()->numRows($result) > 0) {
-            continue; // Permission already exists, skip
-        }
-
         $query = sprintf(
             'INSERT INTO %sfaqdata_%s (record_id, %s_id) VALUES (%d, %d)',
             Database::getTablePrefix(),

Replace with a single upfront query:

// Fetch existing IDs in one query
$existingIds = [];
$checkQuery = sprintf(
    'SELECT %s_id FROM %sfaqdata_%s WHERE record_id = %d',
    $mode,
    Database::getTablePrefix(),
    $mode,
    $faqId,
);
$result = $this->configuration->getDb()->query($checkQuery);
if ($result && $this->configuration->getDb()->numRows($result) > 0) {
    while ($row = $this->configuration->getDb()->fetchObject($result)) {
        $existingIds[] = (int) $row->{$mode . '_id'};
    }
}

foreach ($ids as $id) {
    if (in_array($id, $existingIds, true)) {
        continue;
    }
    // ... INSERT ...
}

tests/phpMyFAQ/Entity/PushSubscriptionEntityTest.php (1)

112-132: testFluentInterface omits assertions for endpointHash and createdAt.

The test chains all setters but only asserts a subset of fields. Consider adding the missing assertions for completeness:

💡 Proposed addition

         $this->assertEquals('authtoken', $this->entity->getAuthToken());
         $this->assertEquals('aesgcm', $this->entity->getContentEncoding());
+        $this->assertEquals(hash('sha256', 'https://example.com/push'), $this->entity->getEndpointHash());
+        $this->assertEquals($date, $this->entity->getCreatedAt());
     }

nginx.conf (1)

82-89: Consider using default_type instead of add_header Content-Type.

The Docker nginx config (.docker/nginx/default.conf) uses default_type application/javascript for the same /sw.js block, which is the correct approach. Using add_header Content-Type can result in a duplicate Content-Type header alongside the one nginx derives from its MIME type mapping, since add_header appends response headers rather than replacing core ones.

Also note that add_header in a location block suppresses all inherited add_header directives (e.g., the X-Frame-Options SAMEORIGIN on Line 73). This is fine for a service worker script but worth being aware of.

Align with the Docker nginx config

     # Service Worker - no caching, proper headers
     location = /sw.js {
-        add_header Content-Type "application/javascript; charset=utf-8";
+        default_type application/javascript;
         add_header Service-Worker-Allowed "/";
         add_header Cache-Control "no-cache, no-store, must-revalidate";
         add_header X-Content-Type-Options "nosniff";
         try_files $uri =404;
     }

phpmyfaq/admin/assets/src/api/push.test.ts (1)

12-17: vi.restoreAllMocks() won't restore globalThis.fetch after direct assignment.

Directly assigning globalThis.fetch = vi.fn(...) means restoreAllMocks() on Line 6 cannot restore the original reference — it only restores spies created via vi.spyOn. Consider using vi.spyOn instead, or save/restore the original in beforeEach/afterEach.

Use vi.spyOn for proper restoration

-      globalThis.fetch = vi.fn(() =>
-        Promise.resolve({
-          ok: true,
-          json: () => Promise.resolve(mockResponse),
-        } as Response)
-      );
+      vi.spyOn(globalThis, 'fetch').mockResolvedValue({
+        ok: true,
+        json: () => Promise.resolve(mockResponse),
+      } as Response);

And similarly for the error test:

-      const mockError = new Error('Fetch failed');
-      globalThis.fetch = vi.fn(() => Promise.reject(mockError));
+      const mockError = new Error('Fetch failed');
+      vi.spyOn(globalThis, 'fetch').mockRejectedValue(mockError);

tests/phpMyFAQ/Setup/Installation/DatabaseSchemaTest.php (1)

39-51: Consider asserting the new table name in testGetTableNamesReturnsCorrectNames.

The test verifies specific table names like faqchat_messages and faqcustompages but doesn't include faqpush_subscriptions. Adding it would keep coverage consistent with the other recently added tables.

Suggested addition

         $this->assertContains('faqchat_messages', $names);
         $this->assertContains('faqcustompages', $names);
         $this->assertContains('faqseo', $names);
+        $this->assertContains('faqpush_subscriptions', $names);

phpmyfaq/assets/templates/default/index.twig (1)

231-248: Push opt-in banner looks well-structured.

Good use of d-none default with progressive JS enhancement. Gating on both pushEnabled and isUserLoggedIn is correct — push subscriptions should only be offered to authenticated users.

One minor note: the inline z-index: 1050 matches Bootstrap's modal layer. If a modal opens while the banner is visible, they could visually overlap. Consider using z-index: 1040 (below modals) or a utility class to avoid potential stacking conflicts.

phpmyfaq/assets/src/api/push.test.ts (1)

1-4: subscribePush is not covered by any test.

The production module exports subscribePush (per the AI summary), but it is neither imported nor tested here. Consider adding a test for the subscribe flow — it's the most important endpoint in the push lifecycle.

phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PushController.php (1)

33-33: Route action name uses hyphens instead of dots.

Other admin API routes use dot-separated names (e.g., admin.api.faq.create, admin.api.faqs.sticky.order). This route uses admin.api.push.generate-vapid-keys with hyphens in the action segment. Consider admin.api.push.generateVapidKeys or similar for consistency.

Based on learnings: "Admin API routes should follow the naming convention: admin.api.{resource}.{action}"

tests/phpMyFAQ/Push/PushSubscriptionRepositoryTest.php (1)

70-98: Consider adding tests with actual subscription results.

getByUserId and getAll are only tested for the empty-array case. The mapRowToEntity mapping logic (which hydrates PushSubscriptionEntity from a DB row) is never exercised. A test with fetchObject returning a mock row object would catch mapping bugs.

phpmyfaq/admin/assets/src/configuration/webpush.ts (1)

62-67: Static analysis innerHTML warnings are false positives here — all values are hardcoded string literals with no user-controlled input. No XSS risk.

However, the button label and notification messages (Lines 66, 73, 85, 87, 90, 93) are English-only hardcoded strings. If the admin interface supports localization, these should go through the translation system.

phpmyfaq/src/phpMyFAQ/Entity/PushSubscriptionEntity.php (1)

26-40: Properties are public despite having getters/setters.

All properties are declared public while the class provides getters and setters. This allows bypassing the setter API (e.g., $entity->authToken = 'x' skips the #[\SensitiveParameter] annotation context). Consider using private or protected visibility to enforce use of the setter API, especially for sensitive fields like authToken.

♻️ Suggested visibility change

 class PushSubscriptionEntity
 {
-    public int $id;
+    private int $id;
 
-    public int $userId;
+    private int $userId;
 
-    public string $endpoint;
+    private string $endpoint;
 
-    public string $endpointHash;
+    private string $endpointHash;
 
-    public string $publicKey;
+    private string $publicKey;
 
-    public string $authToken;
+    private string $authToken;
 
-    public ?string $contentEncoding;
+    private ?string $contentEncoding;
 
-    public DateTimeImmutable $createdAt;
+    private DateTimeImmutable $createdAt;

phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/PushController.php (1)

70-77: Redundant endpointHash computation — repository recomputes it.

The entity's endpointHash is set here (Line 74), but PushSubscriptionRepository::save() (see PushSubscriptionRepository.php Line 43) recomputes it from the endpoint, ignoring the entity's hash. This is confusing and error-prone if the two diverge. Consider either removing the hash computation here or making the repository use the entity's hash.

phpmyfaq/src/phpMyFAQ/Push/PushSubscriptionRepository.php (1)

123-160: Duplicated fetch-loop logic in getByUserId and getAll.

Both methods repeat the same while ($row) { ... $row = $db->fetchObject($result); } pattern. Consider extracting a private helper to reduce duplication.

tests/phpMyFAQ/Push/WebPushServiceTest.php (1)

38-62: Mock returns 'true'/'false' strings, but real Configuration::get() converts them to booleans.

Looking at Configuration::get() (from the relevant snippets), it uses a match to convert 'true' → true and 'false' → false. So in production, isEnabled() will receive a boolean, not a string. The isEnabled() method handles both (=== 'true' || === true), so these tests pass—but they exercise the string-comparison branch that won't occur in practice. Consider using boolean return values in the mock to better reflect real behavior.

phpmyfaq/assets/src/api/push.ts (2)

82-99: unsubscribePush reuses PushSubscribeResponse as return type.

The return type PushSubscribeResponse is semantically misleading for an unsubscribe operation. Consider introducing a shared PushActionResponse type (or similar) to avoid the naming confusion.

---

30-46: Unnecessary Content-Type: application/json header on GET requests.

Lines 34–36 (and similarly Lines 105–107 in getPushStatus) set a Content-Type header on GET requests that have no body. This is harmless but unnecessary.

phpmyfaq/assets/src/push/index.ts (2)

144-153: Consider reversing unsubscribe order: notify server before local unsubscribe.

If unsubscribePush(endpoint) fails after currentSubscription.unsubscribe() succeeds (line 147–148), the browser is unsubscribed but the server retains a stale record. While the server will eventually purge it on the next send attempt, reversing the order would let you abort cleanly on server failure without orphaning the local subscription.

♻️ Suggested reorder

       if (isSubscribed && currentSubscription) {
         const endpoint = currentSubscription.endpoint;
-        await currentSubscription.unsubscribe();
         await unsubscribePush(endpoint);
+        await currentSubscription.unsubscribe();
         isSubscribed = false;

---

229-235: Fragile DOM traversal: parentElement?.parentElement? is brittle.

Line 231 walks up two parent nodes to hide the UCP section. Any template change will silently break this. Prefer closest() with a known selector.

♻️ Suggested fix

       if (!vapidResponse.enabled || !vapidResponse.vapidPublicKey) {
         if (ucpButton) {
-          ucpButton.parentElement?.parentElement?.classList.add('d-none');
+          ucpButton.closest('.pmf-push-toggle-container')?.classList.add('d-none');
         }

Add the .pmf-push-toggle-container class (or a suitable selector) to the wrapping element in the template.

phpmyfaq/src/phpMyFAQ/Push/WebPushService.php (1)

114-124: VAPID subject is fetched from config twice; extract to a local variable.

$this->configuration->get('push.vapidSubject') is called on both lines 117 and 119. A local variable is cleaner and avoids the double lookup.

♻️ Suggested simplification

         $auth = [
             'VAPID' => [
-                'subject' =>
-                    $this->configuration->get('push.vapidSubject') !== ''
-                    && $this->configuration->get('push.vapidSubject') !== null
-                        ? $this->configuration->get('push.vapidSubject')
-                        : 'mailto:' . $this->configuration->getAdminEmail(),
+                'subject' => !empty($vapidSubject = $this->configuration->get('push.vapidSubject'))
+                    ? $vapidSubject
+                    : 'mailto:' . $this->configuration->getAdminEmail(),
                 'publicKey' => $this->configuration->get('push.vapidPublicKey'),
                 'privateKey' => $this->configuration->get('push.vapidPrivateKey'),
             ],
         ];

Or, for readability:

$vapidSubject = $this->configuration->get('push.vapidSubject');
$auth = [
    'VAPID' => [
        'subject' => !empty($vapidSubject) ? $vapidSubject : 'mailto:' . $this->configuration->getAdminEmail(),
        // ...
    ],
];

tests/phpMyFAQ/Setup/Migration/QueryBuilder/TableBuilderTest.php (1)

338-365: Formatting looks good; consider aligning the sibling test for consistency.

The multi-line chain style on lines 355–360 is an improvement for readability. However, the nearly identical chain in testSqliteAutoIncrementDoesNotDuplicatePrimaryKey (line 341) is still on a single line. Consider reformatting that one as well so the two related tests are visually consistent.

Also, the trailing comment on line 360 (// This should be ignored when autoIncrement is used) describes the behavior of ->primaryKey('id'), not ->build(). Moving it to a standalone line above the chain (or next to ->primaryKey('id')) would be clearer.

♻️ Suggested alignment with sibling test

     public function testSqliteAutoIncrementDoesNotDuplicatePrimaryKey(): void
     {
         $sqliteBuilder = new TableBuilder(new SqliteDialect());
-        $sql = $sqliteBuilder->table('test', false)->autoIncrement('id')->varchar('name', 100)->build();
+        $sql = $sqliteBuilder
+            ->table('test', false)
+            ->autoIncrement('id')
+            ->varchar('name', 100)
+            ->build();

     public function testAutoIncrementSkipsExplicitPrimaryKey(): void
     {
         $sqliteBuilder = new TableBuilder(new SqliteDialect());
+        // primaryKey('id') should be ignored when autoIncrement is used
         $sql = $sqliteBuilder
             ->table('test', false)
             ->autoIncrement('id')
             ->varchar('name', 100)
             ->primaryKey('id')
-            ->build(); // This should be ignored when autoIncrement is used
+            ->build();

phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php (1)

269-276: Duplicated FAQ URL construction pattern.

The sprintf pattern for building the public FAQ URL is identical to lines 202–208 (open question answered flow). Consider extracting a small helper to avoid maintaining the same URL template in two places within this method.

♻️ Suggested helper extraction

+    // Helper to build public FAQ URL (could also be a method on FaqEntity or a utility)
+    $buildFaqUrl = static fn (int $categoryId, FaqEntity $faq, string $baseUrl): string => sprintf(
+        '%scontent/%d/%d/%s/%s.html',
+        $baseUrl,
+        $categoryId,
+        $faq->getId(),
+        $faq->getLanguage(),
+        TitleSlugifier::slug($faq->getQuestion()),
+    );

Then replace both occurrences (lines 202–208 and 269–275) with:

$faqUrl = $buildFaqUrl($categories[0], $faqData, $this->configuration->getDefaultUrl());

phpmyfaq/admin/assets/src/configuration/webpush.ts (1)

86-86: Consider i18n for user-facing strings.

The notification messages ("VAPID keys have been generated successfully", "Failed to generate VAPID keys", "Generate VAPID Keys", "Generating...") are hardcoded in English. If the admin backend supports localization, these should use the translation system.

phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/PushController.php (1)

52-52: Authenticated endpoint route names use api.private.* instead of the frontend convention.

Per coding guidelines, frontend routes should follow public.{resource}.{action}. These three authenticated routes use the api.private.push.* pattern. Consider aligning them — e.g. public.push.subscribe, public.push.unsubscribe, public.push.status — or establishing a clear private.{resource}.{action} convention for authenticated frontend API routes. As per coding guidelines, "Frontend routes should follow the naming convention: public.{resource}.{action}".

Also applies to: 94-94, 125-125

phpmyfaq/src/phpMyFAQ/Push/WebPushService.php (1)

125-174: sendToSubscriptions is well-structured overall.

Good use of the try/catch around the entire notification flow, expired-endpoint cleanup via flush reports, and the fallback to getAdminEmail() for the VAPID subject. A couple of minor observations:

• The push.vapidSubject config value is fetched three times (Lines 135, 136, 137). Assigning it to a local variable would be cleaner and slightly more efficient.
• Notification sending is synchronous; for large subscriber counts, consider documenting that this should be dispatched to a background job. Not blocking for now, but worth noting for future scale.

♻️ Minor: cache the config lookup

+        $vapidSubject = $this->configuration->get('push.vapidSubject');
         $auth = [
             'VAPID' => [
                 'subject' =>
-                    $this->configuration->get('push.vapidSubject') !== ''
-                    && $this->configuration->get('push.vapidSubject') !== null
-                        ? $this->configuration->get('push.vapidSubject')
+                    !empty($vapidSubject)
+                        ? $vapidSubject
                         : 'mailto:' . $this->configuration->getAdminEmail(),

phpmyfaq/src/phpMyFAQ/Push/PushSubscriptionRepository.php (2)

40-89: TOCTOU race in the upsert: concurrent requests for the same endpoint can cause a duplicate-key failure.

The save() method performs a SELECT to check existence, then either UPDATEs or INSERTs. Between these two queries, a concurrent request with the same endpoint could insert a row, causing the second INSERT to fail (assuming a unique index on endpoint_hash). For push subscriptions the practical risk is low, but be aware this can surface under load.

A database-level INSERT … ON DUPLICATE KEY UPDATE (MySQL) or INSERT … ON CONFLICT (PostgreSQL) would be the robust fix, though the multi-DB abstraction layer may not support it. At minimum, consider wrapping the check-and-write in a transaction.

---

127-146: Duplicated fetch-loop pattern across three methods.

getByUserId, getByUserIds, and getAll share identical row-fetching logic (query → check false → while-loop → mapRowToEntity). Consider extracting a private helper like fetchSubscriptions(string $query): array.

♻️ Proposed helper extraction

+    /**
+     * `@return` PushSubscriptionEntity[]
+     */
+    private function fetchSubscriptions(string $query): array
+    {
+        $db = $this->configuration->getDb();
+        $result = $db->query($query);
+        if ($result === false) {
+            return [];
+        }
+
+        $subscriptions = [];
+        $row = $db->fetchObject($result);
+        while ($row) {
+            $subscriptions[] = $this->mapRowToEntity($row);
+            $row = $db->fetchObject($result);
+        }
+
+        return $subscriptions;
+    }

Then each method simplifies to building the query string and calling return $this->fetchSubscriptions($query);.

Also applies to: 154-178, 185-204

phpmyfaq/src/phpMyFAQ/Push/PushSubscriptionRepository.php (3)

40-89: save() recomputes endpointHash but ignores the value already set on the entity.

Line 43 computes $endpointHash = hash('sha256', $entity->getEndpoint()), which is correct for the DB operation. However, this means the entity's own endpointHash property (set by the caller via setEndpointHash()) is silently ignored. If a caller sets a different hash on the entity, it will be overwritten in the DB without any indication. Consider either (a) always using $entity->getEndpointHash() (trusting the caller), or (b) setting the computed hash back on the entity so it stays consistent after save.

Option (b) — keep entity in sync

     $endpointHash = hash('sha256', $entity->getEndpoint());
+    $entity->setEndpointHash($endpointHash);

---

202-221: getAll() fetches every subscription without a LIMIT.

If the subscriber base grows large, this will load all rows into memory in a single query. This is used by WebPushService to send notifications, so it could become a bottleneck. Consider adding pagination or a batched iterator, or at minimum a configurable upper bound.

---

144-163: Duplicated fetch-loop pattern across three methods.

getByUserId, getByUserIds, and getAll each repeat the identical while ($row) { … fetchObject } loop. A small private helper would reduce duplication.

♻️ Extract helper

+    /**
+     * Fetches all rows from a result set and maps them to entities.
+     *
+     * `@return` PushSubscriptionEntity[]
+     */
+    private function fetchAllEntities($result): array
+    {
+        $db = $this->configuration->getDb();
+        $subscriptions = [];
+        $row = $db->fetchObject($result);
+        while ($row) {
+            $subscriptions[] = $this->mapRowToEntity($row);
+            $row = $db->fetchObject($result);
+        }
+        return $subscriptions;
+    }

Then each method simplifies to:

return $this->fetchAllEntities($result);

Also applies to: 171-195, 202-221

phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/PushController.php (3)

37-47: VAPID public key endpoint returns key even when push is disabled.

When $webPushService->isEnabled() is false, the endpoint still returns the VAPID public key (which may be empty or a stale value). Consider returning null for the key when push is disabled to avoid leaking configuration details unnecessarily and to give the client a clearer signal.

♻️ Suggested tweak

         return $this->json([
             'enabled' => $webPushService->isEnabled(),
-            'vapidPublicKey' => $webPushService->getVapidPublicKey(),
+            'vapidPublicKey' => $webPushService->isEnabled() ? $webPushService->getVapidPublicKey() : null,
         ], Response::HTTP_OK);

---

63-66: FILTER_SANITIZE_SPECIAL_CHARS may mangle Web Push key material.

FILTER_SANITIZE_SPECIAL_CHARS HTML-encodes characters like &, ', ", <, >. While base64url-encoded keys ([A-Za-z0-9_-=]) shouldn't typically contain these, using an HTML-oriented sanitizer on cryptographic key data is semantically wrong and fragile. A validation filter (e.g., regex allowlist for base64url characters) would be more appropriate than a sanitizer that silently transforms input.

---

37-37: Route naming uses api.public.* / api.private.* — deviates from conventions.

The coding guidelines state frontend routes should follow public.{resource}.{action}, and learnings indicate API routes should follow api.{resource}.{action}. The current names (api.public.push.vapid-public-key, api.private.push.subscribe, etc.) blend both patterns with an extra public/private qualifier. If this is an intentional convention for distinguishing authenticated vs. unauthenticated API endpoints in this project, it should be documented. Otherwise, align with one convention.

As per coding guidelines, "Frontend routes should follow the naming convention: public.{resource}.{action}". Based on learnings, "API routes should follow the naming convention: api.{resource}.{action}".

Also applies to: 52-52, 94-94, 126-126

phpmyfaq/src/phpMyFAQ/Notification.php (1)

307-324: Method exists and is properly typed — optional guard remains valid.

User::getSuperAdminIds() at line 970 in User.php is correctly defined as public static function getSuperAdminIds(Configuration $configuration): array and properly returns int[]. The usage at line 315 is correct.

The optional guard against empty $adminUserIds remains a reasonable optimization. If both no category owner ($userId <= 0) and no superadmins are found, sendToUsers() is called with an empty array, which may result in unnecessary processing. Consider adding:

if (empty($adminUserIds)) {
    return;
}

This prevents calling the web push service when there are no recipients.

phpmyfaq/src/phpMyFAQ/Push/PushSubscriptionRepository.php (1)

154-160: Consider extracting the repeated fetch loop into a private helper.

The fetch-object-while-loop pattern is duplicated across getByUserId, getByUserIds, and getAll. A small private method would reduce repetition and centralize the mapping logic.

♻️ Suggested helper extraction

+    /**
+     * `@return` PushSubscriptionEntity[]
+     */
+    private function fetchAll($result): array
+    {
+        $subscriptions = [];
+        $row = $this->configuration->getDb()->fetchObject($result);
+        while ($row) {
+            $subscriptions[] = $this->mapRowToEntity($row);
+            $row = $this->configuration->getDb()->fetchObject($result);
+        }
+        return $subscriptions;
+    }

Each caller then becomes:

-        $subscriptions = [];
-
-        $row = $db->fetchObject($result);
-        while ($row) {
-            $subscriptions[] = $this->mapRowToEntity($row);
-            $row = $db->fetchObject($result);
-        }
-
-        return $subscriptions;
+        return $this->fetchAll($result);

Also applies to: 186-192, 212-218

phpmyfaq/src/phpMyFAQ/Push/PushSubscriptionRepository.php (1)

157-165: Consider extracting the fetch loop into a private helper.

The fetch-and-map loop is repeated verbatim in getByUserId, getByUserIds, and getAll. A small helper would reduce duplication:

Example helper

private function fetchAll($result): array
{
    $subscriptions = [];
    $row = $this->configuration->getDb()->fetchObject($result);
    while ($row) {
        $subscriptions[] = $this->mapRowToEntity($row);
        $row = $this->configuration->getDb()->fetchObject($result);
    }
    return $subscriptions;
}

Each caller then becomes:

         $result = $db->query($query);
         if ($result === false) {
             return [];
         }
-        $subscriptions = [];
-        $row = $db->fetchObject($result);
-        while ($row) {
-            $subscriptions[] = $this->mapRowToEntity($row);
-            $row = $db->fetchObject($result);
-        }
-        return $subscriptions;
+        return $this->fetchAll($result);

Also applies to: 189-197, 215-223